Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix][broker] Fix NPE after publishing a tombstone to the service unit channel #22859

Merged

Conversation

BewareMyPower
Copy link
Contributor

Motivation

NPE will happen in UnloadManager#handleEvent after #22743. It's because the Init state is always associated with a null ServiceUnitStateData.

java.lang.NullPointerException: Cannot invoke "org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateData.force()" because "data" is null
        at org.apache.pulsar.broker.loadbalance.extensions.manager.UnloadManager.handleEvent(UnloadManager.java:204) ~[classes/:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.StateChangeListeners.lambda$notify$3(StateChangeListeners.java:74) ~[classes/:?]
        at java.base/java.util.concurrent.CopyOnWriteArrayList.forEach(CopyOnWriteArrayList.java:807) ~[?:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.StateChangeListeners.notify(StateChangeListeners.java:72) ~[classes/:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelImpl.handleInitEvent(ServiceUnitStateChannelImpl.java:902) ~[classes/:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelImpl.handle(ServiceUnitStateChannelImpl.java:715) ~[classes/:?]

Modifications

In UnloadManager#handleEvent, assume data is null and call complete directly. Fix UnloadManagerTest, which passes a non-null ServiceUnitStateData and Init to handleEvent.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

…t channel

### Motivation

NPE will happen in `UnloadManager#handleEvent` after
apache#22743. It's because the `Init`
state is always associated with a null `ServiceUnitStateData`.

```
java.lang.NullPointerException: Cannot invoke "org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateData.force()" because "data" is null
        at org.apache.pulsar.broker.loadbalance.extensions.manager.UnloadManager.handleEvent(UnloadManager.java:204) ~[classes/:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.StateChangeListeners.lambda$notify$3(StateChangeListeners.java:74) ~[classes/:?]
        at java.base/java.util.concurrent.CopyOnWriteArrayList.forEach(CopyOnWriteArrayList.java:807) ~[?:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.StateChangeListeners.notify(StateChangeListeners.java:72) ~[classes/:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelImpl.handleInitEvent(ServiceUnitStateChannelImpl.java:902) ~[classes/:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelImpl.handle(ServiceUnitStateChannelImpl.java:715) ~[classes/:?]
```

### Modifications

In `UnloadManager#handleEvent`, assume `data` is null and call
`complete` directly. Fix `UnloadManagerTest`, which passes a non-null
`ServiceUnitStateData` and `Init` to `handleEvent`.
@BewareMyPower BewareMyPower added this to the 3.4.0 milestone Jun 6, 2024
@BewareMyPower BewareMyPower self-assigned this Jun 6, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 6, 2024
@heesung-sn
Copy link
Contributor

Thank you for catching this.

@heesung-sn heesung-sn closed this Jun 6, 2024
@heesung-sn heesung-sn reopened this Jun 6, 2024
@heesung-sn
Copy link
Contributor

@BewareMyPower
Can you fix the check style error?

[INFO] There is 1 error reported by Checkstyle 10.14.2 with /home/runner/work/pulsar/pulsar/buildtools/src/main/resources/pulsar/checkstyle.xml ruleset.
Error:  src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java:[25,1] (imports) ImportOrder: Import com.google.common.base.Preconditions.checkArgument appears after other imports that it should precede

@BewareMyPower
Copy link
Contributor Author

The checkstyle issue is fixed now.

@BewareMyPower BewareMyPower merged commit 9326a08 into apache:master Jun 7, 2024
50 of 51 checks passed
@BewareMyPower BewareMyPower deleted the bewaremypower/extensible-lb-npe branch June 7, 2024 03:09
heesung-sn pushed a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn pushed a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn pushed a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
…t channel (apache#22859)

(cherry picked from commit 9326a08)
(cherry picked from commit 6eac7e5)
heesung-sn pushed a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn pushed a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn pushed a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn pushed a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
…t channel (apache#22859)

(cherry picked from commit 9326a08)
(cherry picked from commit 6eac7e5)
heesung-sn pushed a commit to heesung-sn/pulsar that referenced this pull request Jun 27, 2024
…t channel (apache#22859)

(cherry picked from commit 9326a08)
(cherry picked from commit 6eac7e5)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 27, 2024
…t channel (apache#22859)

(cherry picked from commit 9326a08)
(cherry picked from commit 6eac7e5)
(cherry picked from commit 9ffbffc)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 1, 2024
…t channel (apache#22859)

(cherry picked from commit 9326a08)
(cherry picked from commit 6eac7e5)
(cherry picked from commit 9ffbffc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants